Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support windows command resolution #1290

Merged
merged 3 commits into from
Sep 28, 2016
Merged

Conversation

smurawski
Copy link
Contributor

Adds search by PATHEXT extensions to the command search of PATH.

Signed-off-by: Steven Murawski steven.murawski@gmail.com

@thesentinels
Copy link
Contributor

Thanks for the pull request! Here is what will happen next:

  1. Your PR will be reviewed by the maintainers
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

if current_candidate.is_file() {
return Some(current_candidate);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like in Ruby, the last line of a function is the return value in Rust. Short-circuiting the function with return Some(T) on 228 is great, but we should fall through to a None below this line. That would allow you to remove the ; on 233 & the explicit call to return None on 232.

These are small suggestions but they'll make the function control flow read a bit easier and be more "rust-like"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely appreciate the feedback on making it more rust-like!

@@ -201,13 +201,39 @@ pub fn find_command(command: &str) -> Option<PathBuf> {
if candidate.is_file() {
return Some(candidate);
}
else {
match find_command_with_pathext(&candidate) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The match here would be unnecessary if the ; on line 202 was removed along with the None on line 211. find_command_with_pathext already returns an Option so if you just call that without the ; appended to the end you'll get the return value you're looking for!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback!

I think that would only work for the first path in the path variable checked. The second part of the match None => {} is what lets things proceed on to the next path to check.

I'm going to write some tests to validate the behavior.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, you're right. I looked quick and didn't notice we were in an iterator and trying to short-circuit out of it in this function as well

@smurawski smurawski force-pushed the smurawski/win_studio branch 3 times, most recently from 1c2a421 to 2ab1c98 Compare September 28, 2016 17:32
Signed-off-by: Steven Murawski <steven.murawski@gmail.com>
Signed-off-by: Steven Murawski <steven.murawski@gmail.com>
@smurawski smurawski force-pushed the smurawski/win_studio branch 2 times, most recently from 093fab6 to d032ac1 Compare September 28, 2016 18:12
@smurawski
Copy link
Contributor Author

@reset I've made some changes to the structure and behavior and added a bunch of tests. Mind giving it another 👀 ?

Copy link
Collaborator

@reset reset left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just the one comment to address regarding the debug output

@@ -199,7 +241,13 @@ pub fn find_command(command: &str) -> Option<PathBuf> {
for path in env::split_paths(&paths) {
let candidate = PathBuf::from(&path).join(command);
if candidate.is_file() {
println!("Found {:?}", candidate);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it's either debug output or something left over from iterating. If it's debug output let's use debug!() instead of println!() and if it's just left over from iterating remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, thought I killed all those. I'll remove it.

}
}

#[cfg(target_os="windows")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a neat way to group tests in a platform dependent way. I hadn't seen us use submodules in a test module yet but this is a nice pattern

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started using the modules as a way to group the tests to keep the function names shorter, but it became handy when I needed to make one windows only :)

Signed-off-by: Steven Murawski <steven.murawski@gmail.com>
@reset
Copy link
Collaborator

reset commented Sep 28, 2016

@thesentinels approve

@thesentinels
Copy link
Contributor

🤘 I am testing your branch against master before merging it. We do this to ensure that the master branch is never failing tests.

@thesentinels
Copy link
Contributor

:neckbeard: Travis CI has started testing this PR.

@thesentinels
Copy link
Contributor

💖 Travis CI reports this PR passed.

It always makes me feel nice when humans approve of one anothers work. I'm merging this PR now.

I just want you and the contributor to answer me one question:

gif-keyboard-3280869874741411265

@thesentinels thesentinels merged commit 278cc57 into master Sep 28, 2016
@thesentinels thesentinels deleted the smurawski/win_studio branch September 28, 2016 21:48
@smurawski
Copy link
Contributor Author

progress on #1192

@eeyun eeyun added C-feature and removed Feature labels Jun 6, 2017
@christophermaier christophermaier added Type: Feature Issues that describe a new desired feature and removed C-feature labels Jul 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Issues that describe a new desired feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants